Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hono/jwk): JWK Auth Middleware #3826

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

feat(hono/jwk): JWK Auth Middleware #3826

wants to merge 24 commits into from

Conversation

Beyondo
Copy link
Contributor

@Beyondo Beyondo commented Jan 13, 2025

Hello, I recently needed a JWK middleware for my projects, but I figured contributing it to hono has the potential to save me and others a lot of time.

Middleware Features:

  • Set options.keys to a static array of public keys JsonWebKey[] in code.
  • Set options.keys to an async function that returns a Promise<JsonWebKey[]> for flexibility
  • Set options.jwks_uri to fetch JWKs from a URI, after which it appends those fetched keys to provided keys if any
  • Set an optional init parameter (only used for jwks_uri)—useful if your host supports caching through custom init options.

Added extra:

  • Added JwtHeaderRequiresKid exception. Since the middleware requires presence of a kid field in the header in order to select the correct key.
  • Added Jwt.verifyFromJwks util function (batteries included).

Addressed issues:

Other code changes:

  • Typescript-extended JsonWebKey to have kid?: string (This is a standard: https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.4)
  • Added kid?: string to TokenHeader
  • Fixed a bug in Jwt.sign(payload, privateKey, alg) where privateKey.alg was always ignored and only alg parameter was considered.

Suggestion: Removal of the default 'HS256' value from Jwt.sign and make alg optional, so that if neither privateKey.alg nor alg is defined, throw an error. My 'philosophy' for this is that it would be misleading for hono to display = 'HS256' when the privateKey can internally have a different algorithm, such as when it is of type JsonWebKey or CryptoKey (Though we only care about JsonWebKey here). Making alg a "fallback" explicit parameter could be better.

Example Usage:

Using jwks_uri

import { jwk } from "hono/jwk";
app.use("/backend-api/*", jwk({ jwks_uri: "https://example-backend.hono.dev/.well-known/jwks.json" }));

Using jwks_uri and an optional init

import { jwk } from "hono/jwk";
app.use("/backend-api/*", jwk({
  jwks_uri: "https://example.hono.dev/.well-known/jwks.json"
}, {
  headers: {
    "My-Custom-Header": "Hello Hono!"
  },
  cf: { cacheTtl: 3600, cacheEverything: true } // Only for Cloudflare Workers
}));

Using keys from an array

[
  {
    "kid": "VpNx_Ar8pXsh50UjqBD_XtYLj4k9Z72iakeOQFJYIpg",
    "kty": "RSA",
    "use": "sig",
    "alg": "RS256",
    "e": "AQAB",
    "n": "4anIpNGZ-C5rJfRty8qQt8wTheFDj9wchPA..."
  },
  {
    "kid": "Rw6zp1oTEbQ9qUIFnhHGadtstXhKYrCmkqgkNEG8uUc",
    "kty": "RSA",
    "use": "sig",
    "alg": "RS256",
    "e": "AQAB",
    "n": "vKetN8i4_ORAWAUVR1pCjVMFkNzwC1Qjy..."
  }
]
import keys from './keys.json';
import { jwk } from "hono/jwk";
app.use("/backend-api/*", jwk({ keys: keys }));

Using keys as an async function (custom logic / caching)

$router.use("/auth/*", jwk({
  keys: async () => {
    /* ... Custom caching logic ... */
    return cached_response.keys;
  }
}));

Tests:

I adapted a lot from JWT's own units tests to make sure some basic JWT validation exists, then added the actual validation for the JWK middleware's own functionality which is mostly: (1) getting keys through various methods, (2) selecting the appropriate JWK based on kid (without fully decoding the JWT—only the header), and then (3) verifying using the utility Jwt.verify. Step 2 and 3 are done by Jwt.verifyFromJwks.

JWK Middleware Test

hono> npx src/middleware/jwk/index.test.ts

 DEV  v2.1.1 V:/Repos/hono
      Coverage enabled with v8

 ✓ src/middleware/jwk/index.test.ts (16)
   ✓ JWK (16)
     ✓ Credentials in header (10)
       ✓ Should not authorize requests with missing access token
       ✓ Should authorize from a static array passed to options.keys (key 1)
       ✓ Should authorize from a static array passed to options.keys (key 2)
       ✓ Should authorize with Unicode payload from a static array passed to options.keys
       ✓ Should authorize from a function passed to options.keys
       ✓ Should authorize from a URI remotely fetched from options.jwks_uri
       ✓ Should not authorize requests with invalid Unicode payload in header
       ✓ Should not authorize requests with malformed token structure in header
       ✓ Should not authorize requests without authorization in nested JWK middleware
       ✓ Should authorize requests with authorization in nested JWK middleware
     ✓ Credentials in cookie (5)
       ✓ Should not authorize requests with missing access token
       ✓ Should authorize from a static array passed to options.keys
       ✓ Should authorize with Unicode payload from a static array passed to options.keys
       ✓ Should not authorize requests with invalid Unicode payload in cookie
       ✓ Should not authorize requests with malformed token structure in cookie
     ✓ Error handling with `cause` (1)
       ✓ Should not authorize

 Test Files  1 passed (1)
      Tests  16 passed (16)

Full Test

hono> npm test
...
...
...
 Test Files  9 passed (9)
      Tests  415 passed | 3 skipped (418)

  • [✅] Add tests (src/middleware/jwk/index.test.ts)
  • [✅] Run tests (Above)
  • [✅] bun run format:fix && bun run lint:fix to format the code (Done)
  • [✅] Add TSDoc/JSDoc to document the code (src/middleware/jwk/jwk.ts)

Feel free to contribute more unit tests, extend and/or modify for the better.

…yAlg fallback, add decodeHeaders utility

- Added "kid" (Key ID) for TokenHeader
- Fixed Jwt.sign() ignoring privateKey.alg
- Renamed `alg` parameter to `KEYAlg` to differentiate between privateKey.alg
- Added utility function `decodeHeaders` to decode only JWT headers
Hono JWK Middleware main features:
- Ability to provide a list of public JWKs to the keys parameter as a simple javascript array []
- Ability to provide a URL in the jwks_uri parameter to fetch keys from + an optional RequestInit (useful for caching if your cloud provider has a modified fetch that supports it, or if you simply want to modify the request)
- Ability to provide an async function that returns an array to the keys parameter instead of a direct array, so that it is possible to implement own custom caching layer without a separate middleware
- Allows setting a keys directory for multi-key auth systems
- Allows auth endpoints to be always updated with the Auth provider's public JWKs directory (often `.well-known/jwks.json`) which makes key rotations without disruptions possible

Todo:
- More tests.
Added /auth-keys-fn/* & /.well-known/jwks.json testing endpoints to the router.
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 77.30061% with 37 lines in your changes missing coverage. Please review.

Project coverage is 91.49%. Comparing base (2ead4d8) to head (582334c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/middleware/jwk/jwk.ts 76.27% 28 Missing ⚠️
src/utils/jwt/jwt.ts 83.33% 6 Missing ⚠️
src/utils/jwt/types.ts 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3826      +/-   ##
==========================================
- Coverage   91.71%   91.49%   -0.22%     
==========================================
  Files         160      162       +2     
  Lines       10195    10355     +160     
  Branches     2885     3034     +149     
==========================================
+ Hits         9350     9474     +124     
- Misses        844      880      +36     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Beyondo
Copy link
Contributor Author

Beyondo commented Jan 14, 2025

Note that the test code jwk/index.test.ts was 519 lines so it inflated the actual aggregated changes of this PR, + jsdoc comments, and + the keys.test.json file and so on. The core changes are probably < 200 lines and thus much more mangeable to review.

I removed some commented out tests to give a clearer picture (Now at 307 lines of test code). But they did remind me—we need some signed cookie unit tests if we're gonna make sure to support everything in jwt/index.test.ts too. Though I don't think it's exactly a priority and could be implemented later.

@yusukebe
Copy link
Member

Hi @Beyondo

Thanks! I'll check this later.

@Beyondo
Copy link
Contributor Author

Beyondo commented Jan 14, 2025

@yusukebe No problem! I originally named the new interface ExtendedJsonWebKey, though I do think HonoJsonWebKey might also work better since the only addition is the optional kid?, making it function just like the normal JsonWebKey if needed, so we might need some opinion on this.

Take your time! I'm using the fork temporarily, so no rush at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants